[DSL] Add extra commands/abilities to DSL scripts/rules#5481
[DSL] Add extra commands/abilities to DSL scripts/rules#5481Nadahar wants to merge 8 commits intoopenhab:mainfrom
Conversation
|
Also, consider the specific new commands I've implemented as more of a suggestion. This can be tweaked as desired. @rkoshak Perhaps you have something to add here? |
|
The itest fails because of some unresolved reference. I'm trying to figure it out, but if anybody has any hints of how to figure out the problem, it would be appreciated, because I frankly have no idea how to handle the bndrun resolution. |
|
I don't think the itest fails over dependencies, but that the startlevel is insufficient: If I understand it correctly, So, the question is: Is it actually a problem that |
…ty to run and enable/disable rules to DSL scripts Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
I've set the startlevel in the test to 40, which is required for the script engine to start, and it seems to resolve the problem with the test. I'm still uncertain as to whether reaching startlevel 40 before the Are there scripts being executed before startlevel 40 is reached? |
|
Now there's a new test failure, not sure exactly how it is related yet: |
| * @throws IllegalArgumentException If a rule with the specified UID doesn't exist. | ||
| */ | ||
| @ActionDoc(text = "run the rule with the specified UID, condition evaluation setting and context") | ||
| public static Map<String, Object> runRule(String ruleUID, boolean considerConditions, Map<String, Object> context) { |
There was a problem hiding this comment.
It seems weird to make considerConditions the second argument given there is a version of the method that includes the context but doesn't take the condition boolean. That is how it appears in JS. I don't know the other languages.
As an end user who knows nothing of the RuleManager, I would expect the actions to be:
runRule(ruleUID)runRule(ruleUID, context)runRule(ruleUID, context, considerConditions)
There was a problem hiding this comment.
I followed the parameter order of the underlying methods. I'd say that it's not a big deal either way, but that perhaps it would be useful with a runRule(ruleUID, considerConditions) as well?
There was a problem hiding this comment.
When it comes to JS, that's an entirely different thing. It allows that you only specify some parameters, the rest are undefined (?). This means that you must try to guesstimate what parameters people are most likely to drop, and then list those last.
Here, there is no such thing, you must have an exact signature match, so the order doesn't have any significance, unless you use varargs that can't have anything following them (in a sense, in JS, "everything" are varargs of Object). Because collections can often optionally be specified as varargs, you often put them last - even when a varargs method doesn't exist. I could have made a version here where context could be Entry<String, Object>.... In that case, context would have to be last, and my guess is that because that's a thing people get used to, it is somehow intuitive to put the collection last anyway. I didn't do this here because it's awkward to have to create Entry instances for each "context pair" you wish to include, so you might as well provide a Map.
There was a problem hiding this comment.
If we really wanted to go down the road of type-unsafety, I could also let the last parameter be Object..., and then "expect" them to come in pairs where the first was a string and the second some object. The method could then "build" the map and throw an exception if the parameters didn't meet expectations. It might be the most convenient way one could supply context, since you could just supply strings directly in the call, but it "feels" hacky and something you'd do in JS or similar, not in a typed language.
There was a problem hiding this comment.
but that perhaps it would be useful with a runRule(ruleUID, considerConditions) as well?
I'm certainly not against it.
so the order doesn't have any significance,
I just brought up the JS as an example of following a pattern. Given runRule(ruleUID) and runRule(ruleUID, context), the expected next method signature would be runRule(ruleUID, context, considerConditions). Each new signature adds a new argument to the end of the list of arguments. Placing the considerConditions as the second argument violates that expectation.
It's not about technical stuff. It's about making decisions that allow users who are not as familiar with the source code to correctly guess what is correct.
Of course, if you add the newly proposed method there is no more pattern and the order doesn't matter as much.
| * @author Ravi Nadahar - Initial contribution | ||
| */ | ||
| @NonNullByDefault | ||
| public class System { |
There was a problem hiding this comment.
"System" is a pretty generic and almost meaningless name. Would something like "Registries" be feasible? Everything here either exposes or lets users interact with one of the registries.
There was a problem hiding this comment.
The name can be anything, but it does handle more than registries. It also provides OSGi instances and some rule manipulation methods. I chose system because I couldn't really find anything "suitable".
There was a problem hiding this comment.
I also considered names like "OSGi", "Core", "OpenHAB", but they can all be considered "wrong" or misunderstood. The users don't actually have to see this name though. Look at ScriptExecution that creates timers for example. My guess is that nobody knows/cares, because they don't have to qualify the method with the class name.
There was a problem hiding this comment.
What about "Services" or "Utility"? They don't mean much more than "System", but it's hard to find something that means much and also fits the methods.
There was a problem hiding this comment.
What about "Helper"?
There was a problem hiding this comment.
Look at ScriptExecution that creates timers for example. My guess is that nobody knows/cares, because they don't have to qualify the method with the class name.
All of the JSR223 languages have to deal with those class names. It's really only Rules DSL that doesn't. But since this is all for the benefit of Rules DSL in the first place maybe it doesn't matter.
that means much and also fits the methods.
Which brings up something I decided not to mention. Would it make more sense to split this into separate classes, one for rules, one for metadata, one for things and one for Items? Then the naming becomes obvious and clear.
There was a problem hiding this comment.
That could be done, but I'm not sure if there's much to gain by doing it. It would mean more classes that had to be added to the "implicit DSL context", but whether that has any practical consequences I don't know. Probably not.
| * @return The {@link MetadataRegistry}. | ||
| */ | ||
| @ActionDoc(text = "get the metadata registry") | ||
| public static MetadataRegistry getMetadataRegistry() { |
There was a problem hiding this comment.
Even once you gain access to the MetadataRegistry, creating and adding or modifying is a pain because it requires creating a bunch of custom classes. Maybe not for this PR but eventually adding some helper methods like are here for rules would be beneficial.
There was a problem hiding this comment.
I could add more "helper tools" if somebody just give me a hint of what to make (a reference to something else that does something similar)
There was a problem hiding this comment.
As an example, JS provides a metadata on the class it uses to expose the ItemRegistry which lets one access an Items metadata. See https://openhab.github.io/openhab-js/items.metadata.html.
In addition it provides access to those methods on the Item itself. The methods include:
addMetadata(itemOrName, namespace, value, configurationopt, persistopt)getMetadata(itemOrName, namespaceopt)removeMetadata(itemOrName, namespaceopt)replaceMetadata(itemOrName, namespace, value, configurationopt)
itemOrName can be an instance of an Item or the name of an Item. value is a String; empty String is allowed and I think null becomes the empty String. namespace is a String of course. configurationopt is an optional Object though I'd expect it to be a Map in Rules DSL.
persistopt is a new field I'm not familiar with.
There was a problem hiding this comment.
Those methods are quite similar to those I've already created. I could certainly add variants that accepts Item instead of "itemName", if that's easier to provide (if you already have the Item, I guess it's easy to just pass that on).
When in comes to persistopt, that is related to "stuff created by a JS script" and not really relevant here. There's a separate metadata provider that doesn't persist, so that the metadata only exists until the file is unloaded. So, I'm guessing that this option determines whether to store them there or in the "managed metadata provider".
There was a problem hiding this comment.
It's unfortunate that they have called one method replaceMetadata when the underlying method is called updateMetadata. They are the exact same thing, but I prefer to stick to what's underneath, so that it's easier for people to see how it all ties together. I went with "update" instead of "replace".
There was a problem hiding this comment.
. I could certainly add variants that accepts Item instead of "itemName"
I think that is not as useful in DSL as it is in JS.
There was a problem hiding this comment.
I've already done that, but it's done as "extensions", so it becomes:
item.addMetadata(namespace, value [, context etc.])item.getMetadata(namespace)- etc.
Given that Items are magically available in DSL, this should make it very easy to manipulate Item metadata.
|
The current test failure seems to have some complicated timing reason. The following line in Resource resource = resourceSet.createResource(URI.createURI(PREFIX_TMP_MODEL + name));This results in an NPE. The question is why it returns
I'm not sure what this mythical factory is and why it suddenly can't be found, but I assume that it has something to do with |
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
This reverts commit ee8ae1e. Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
I changed the strategy regarding This has resolved the test problems, but comes with the caveat that |
|
In case somebody wants to test it for themselves, this bundle should be all that's needed. |
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
@rkoshak I've added some additional methods for manipulation of metadata. Is that somewhat what you had in mind? I've also reluctantly implemented the varargs "solution" for the maps, so that it's possible to simply call e.g. Since these are just suggestions, I haven't bothered to finish the Javadocs, as I'm sure they'll change, which makes the time I spend writing the documentation a waste. |
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
@rkoshak I've added even more overloads, so not all the metadata methods accept either item name or |
|
@lolodomo There's no a lot of activity here, maybe you're interested in this? This isn't important for me in any way, but seen in light of all the other discussions about DSL versus other scripting languages and the difference in what you can do, I thought that this might be a welcome contribution to close the gap. |
|
Ï've never fully understood the "action" concept in OH. I understand that they are similar to functions/methods, but not what the significance of making things "actions" is. We have for example Why this structure? Is an |
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
I've reorganized The "full versions" are also there, so you can call |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/rules-and-rule-templates-yaml-integration/168568/222 |
|
#5484 adds Running afterwards |
Adding That said, the goal of this PR isn't to make it possible to do those things, but to make it reasonably convenient. As long as imports can be resolved, you can always use This PR has public static @Nullable <T> T getInstance(Class<T> clazz) {
Bundle bundle = FrameworkUtil.getBundle(clazz);
if (bundle != null) {
BundleContext bc = bundle.getBundleContext();
if (bc != null) {
ServiceReference<T> ref = bc.getServiceReference(clazz);
if (ref != null) {
return bc.getService(ref);
}
}
}
return null;
}So, I don't see these two as "competing" in any way. |
|
After By using the function |
Rectification: After |
|
I didn't remember that the services were reference counted, so I'll have to make some modifications to accommodate that. But, as I see it, we can't expect script authors to use try/finally to make sure to release the reference. I've looked a bit in core, and it seems like this is "mishandled" other places as well. If you look at JS Scripting, it does the exact same thing: https://openhab.github.io/openhab-js/osgi.js.html#line38 Since it's unrealistic that script authors can manage the reference counting, I think the best thing to do is to release the reference before returning it. That will work just fine as long as something else keeps a "counted reference" to the instance. In OH, this will almost always be the case, there are other components that reference "everything". As far as I understand, OSGi uses this reference counting to automatically stop services that aren't used by anything. The fact that a service is referenced doesn't prevent an explicit shutdown, so it's not like "a lock", it just prevents automatic shutdown. Since the script acquiring the reference should never have the only reference, releasing it before returning should work fine in practice. In the very unlikely scenario that this isn't the case, the script will fail when it tries to use the service if OSGi has shut it down in the meanwhile. I think that is acceptable, given that it is extremely unlikely to happen, and it should be better than to "leak references". |
4 participants
Add access to various system registries, OSGi instances and the ability to run and enable/disable rules to DSL scripts.
This has been considered "impossible" to do, yet I've met no special obstacles, and it seems to work with my (limited) testing. I'm therefore skeptical that there might be something that I've missed, like additional startup issues caused by
ScriptServiceUtilreferencing additional instances, thus depending on them. But, I've observed no such problems.Additional context/information about the historical problems with this is appreciated, but as far as I can see at the moment, this works without complications.